Reviewed 11 of 11 files at r1.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @lukedirtwalker)

a discussion (no related file):
Add description

go/beacon_srv/internal/beacon/db.go, line 43 at r1 (raw file):

	BeaconSources(ctx context.Context) ([]addr.IA, error)
	// AllRevocations returns all revocations in the database as a channel. The
	// result channel either carries revocations or errors. After sending the

Should we close the channel on first error, or try to serve as many revocations as possible?

go/beacon_srv/internal/beacon/beacondbsqlite/db.go, line 148 at r1 (raw file):

				return
			}
			srev, err := path_mgmt.NewSignedRevInfoFromRaw(rawRev)

In general, this should not happen.
I wonder if we should really return here, or continue.

The receiver will have to drain the channel anyway, so it can just log and skip responses with errors in it. And return hopefully return more parsable revocations.

go/beacon_srv/internal/beacon/beacondbsqlite/db.go, line 482 at r1 (raw file):

	`
	return db.DoInTx(ctx, e.db, func(ctx context.Context, tx *sql.Tx) error {
		existingRev, err := containsNewerRev(ctx, tx, revInfo)

support for upsert would be nice.
Unfortunately we do not have a recent enough sqlite version

go/beacon_srv/internal/beacon/beacondbsqlite/db.go, line 503 at r1 (raw file):

	row := tx.QueryRowContext(ctx, query, revInfo.IA().I, revInfo.IA().A,
		revInfo.IfID, revInfo.RawTimestamp)
	err := row.Scan()

you can chain QueryRowContext and Scan

go/beacon_srv/internal/beacon/beacondbsqlite/db.go, line 524 at r1 (raw file):

	query := `
	DELETE FROM Revocations
	WHERE ExpirationTime < ?

<=

go/beacon_srv/internal/beacon/beacondbsqlite/schema.go, line 50 at r1 (raw file):

		IntfID INTEGER NOT NULL,
		LinkType INTEGER NOT NULL,
		RawTimestamp INTEGER NOT NULL,

This is a bit cryptic, I had to check the code to see what this is.
How about IssuingTime?

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @oncilla)

a discussion (no related file):

Done.

go/beacon_srv/internal/beacon/db.go, line 43 at r1 (raw file):

Done.

go/beacon_srv/internal/beacon/beacondbsqlite/db.go, line 148 at r1 (raw file):

Done.

go/beacon_srv/internal/beacon/beacondbsqlite/db.go, line 482 at r1 (raw file):

+1

go/beacon_srv/internal/beacon/beacondbsqlite/db.go, line 503 at r1 (raw file):

Done.

go/beacon_srv/internal/beacon/beacondbsqlite/db.go, line 524 at r1 (raw file):

No if I understand the Active function on RevInfo correctly the == still means active. But I fixed the comparison in the lookup.

go/beacon_srv/internal/beacon/beacondbsqlite/schema.go, line 50 at r1 (raw file):

Done.

Reviewable status: 8 of 12 files reviewed, 7 unresolved discussions (waiting on @oncilla)

tools/gomocks, line 37 at r2 (raw file):

        (SCION_PACKAGE_PREFIX + "/go/beacon_srv/internal/beaconing",
            "BeaconInserter,BeaconProvider,SegmentProvider"),
        (SCION_PACKAGE_PREFIX + "/go/beacon_srv/internal/ifstate", "RevInserter"),

wrong pr sorry.

Reviewed 4 of 4 files at r2.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @lukedirtwalker)

go/beacon_srv/internal/beacon/db.go, line 53 at r2 (raw file):

	// result channel either carries revocations or errors. The error can
	// either be ErrReadingRows or ErrParse. After a ErrReadingRows occurs the
	// channel is closed, so the result might be incomplete. The channel must

s/, so/and

go/beacon_srv/internal/beacon/beacondbsqlite/db.go, line 137 at r2 (raw file):

	}
	res := make(chan beacon.RevocationOrErr)
	go func() {

I don't think this works well with sqlite.
rows will keep the sqlite internal lock open until they are closed.

For the beacons, I had to read everything in memory first to free the lock as soon as possible and get better performance.

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @oncilla)

go/beacon_srv/internal/beacon/db.go, line 53 at r2 (raw file):

Done.

go/beacon_srv/internal/beacon/beacondbsqlite/db.go, line 137 at r2 (raw file):

Hm as discussed offline:
We don't really use this function except for testing and maybe an external tool so it shouldn't affect performance of the BS.

Reviewed 1 of 2 files at r3.
Reviewable status: complete! all files reviewed, all discussions resolved

Reviewed 3 of 3 files at r4.
Reviewable status: complete! all files reviewed, all discussions resolved

Reviewed 2 of 2 files at r5.
Reviewable status: complete! all files reviewed, all discussions resolved